-
-
Notifications
You must be signed in to change notification settings - Fork 158
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
blosc/CMakeLists.txt: Update Lz4 handling #386
base: main
Are you sure you want to change the base?
Conversation
Lz4 is capable of vendoring its own CMake config module, which is considerd preferable over vendored find modules according to CMake best practices. This patch adds support for importing an LZ4 install detected via its CMake Config module by searching for the targets imported via that module. It then updates the link interface for cblosc static and shared variants to link to their respective LZ4 libraries (or whichevere is available)
set(LIBS ${LIBS} ${STATIC_LIBS}) | ||
endif() | ||
elseif(NOT SHARED_LIBS AND NOT STATIC_LIBS) | ||
# Fallback to cblosc vendored find module |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't the vendored case handled below, after else(LZ4_FOUND)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's the case where you vendor LZ4
in its entirety, this handles the case where the c-blosc
vendored find module for LZ4
is used instead of LZ4
's config module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Different components being vendored, I had hoped the "vendored find module" would be clear enough, but evidently it was not, my apologies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not familiar with libraries providing cmake snippets, i only heard about pkg-config. I'll let someone more familiar with Cmake review. To me, the "genex" code is unreadable and gives vibes of the xz backdoor...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CMake's Config modules are an integral part of a healthy CMake ecosystem, it allows you to export your project in a way that allows consumers to use your project exactly as you intend.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did just notice a mistake in the generator expressions, but that doesn't mean they're a vulnerability...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which is now fixed, forgot to refactor the code from my test project.
Thanks @johnwparent . This contribution seems legitimate to me, but it is going well beyond my knowledge (CMake requires a lot of attention to understand its full capabilities). Besides, I don't fully understand the purpose of this one. But if @kalvdans is fine with it, I'm +1 on merging this. |
Lz4 support producing this config module, so it would be ideal to allow In short, the purpose is to enable compatibility with lz4's config module, while still maintaining the ability to use the vendored find module. Basically the CMake here checks for the target that would be imported by Lz4's config module, and if it's available (i.e. find_package found the config module and used that to provide lz4 instead of The generator expressions look complex, but perform a very simple task. They check for the type of the library the expression is being evaluated in the context of (in this case, either |
Will the code gets smaller if we drop support for the old way of finding the module? |
Technically yes, but you'll lose that functionality, which is still fairly relevant. LZ4 can be built with makefiles or by CMake, so while the CMake derived builds of lz4 can work with this config approach, the makefile based builds cannot. I'd recommend keeping the old find module until Lz4 drops their makefile support. |
How do I test this? I installed liblz4-dev (which on Debian doesn't seem to ship with any cmake config module) and it was detected when I passed Could you @johnwparent please give me some instructions of 1) a dev package that contains the cmake config module, and 2) cmake options that will build a static blosc and 3) dynamically linked blosc. Sorry to ask so much, given your very long description above, but since we don't have any other cmake expert around you'll have to spoonfeed me... |
Lz4 is capable of vendoring its own CMake config module, which is considered preferable over vendored find modules according to CMake best practices.
This patch adds support for importing an LZ4 install detected via its CMake Config module by searching for the targets imported via that module. It then updates the link interface for cblosc static and shared variants to link to their respective LZ4 libraries (or whichever is available).